phar: fix resource leaks on error paths (GH-21798)#21803
phar: fix resource leaks on error paths (GH-21798)#21803iliaal wants to merge 3 commits intophp:PHP-8.4from
Conversation
A uint32_t signature length read from the phar file was passed directly to emalloc() without any upper bound. On a 64-bit build a crafted phar could request a multi-gigabyte heap allocation before any file I/O confirmed the data existed. Cap signature_len at 1 MiB. Real RSA signatures are at most ~512 bytes for 4096-bit keys; 1 MiB is generous enough to avoid false positives while blocking resource exhaustion. Closes phpGH-21798
Phar::offsetGet() calls phar_get_entry_info_dir with allow_dir=1, which may return a heap-allocated temporary directory entry (is_temp_dir=1) for paths that resolve to a virtual directory in the manifest. Three early-exit paths for .phar/stub.php, .phar/alias.txt, and the generic .phar/* prefix all called RETURN_THROWS() before the is_temp_dir cleanup block, leaking the entry and its filename buffer on every rejection. Move the is_temp_dir cleanup before the .phar/* guards so all exit paths release the temporary entry regardless of which rejection fires. Closes phpGH-21798
…r paths phar_add_file opens or creates an entry via phar_get_or_create_entry_data_rw, which increments the entry's reference count and must be balanced by a phar_entry_delref call. Two error paths inside the content-write block jumped to finish: with goto, skipping the phar_entry_delref at line 3714. The finish: label comes after the delref, so both paths leaked the entry reference. Add phar_entry_delref(data) before each goto finish in the short-write and missing-resource branches. Closes phpGH-21798
|
Please split out fix 1 as I am not familiar with the OpenSSL layer whereas the 2 other commits are good. But also, if you are going to submit multiple fixes please do them as multiple PRs. (and don't open an issue if you are just going to fix it immediately this is unnecessary noise for the ones of us that get emails for new issues/PRs) |
I just opened so many PRs for phar, I already felt it was too much so tried to batch to minimize the review overhead and the ones here were pretty small. I will split it as asked and avoid bug reports in the future. |
|
Is the second one even a real issue? It's for directories but neither file is a directory and .phar getting should be blocked anyway IIRC? |
3 participants
Three low-severity resource management fixes filed as #21798.
phar.c - cap OpenSSL signature_len before emalloc
signature_lenis auint32_tread from the phar file without any upper bound. A crafted phar could request a multi-gigabyte heap allocation on a 64-bit build. The 1 MiB cap is generous for real RSA signatures (RSA-8192 produces 1024 bytes) while blocking the resource exhaustion vector.phar_object.c - free is_temp_dir entry before .phar/* rejection in offsetGet
phar_get_entry_info_dirwithallow_dir=1may return a heap-allocated temporary directory entry (is_temp_dir=1). Three early-exit paths for.phar/stub.php,.phar/alias.txt, and the generic.phar/*prefix calledRETURN_THROWS()before theis_temp_dircleanup block, leaking both the entry struct and its filename buffer. Moving the cleanup before the guards fixes all three exit paths.phar_object.c - phar_entry_delref before goto finish in phar_add_file
phar_get_or_create_entry_data_rwincrements the entry reference count; the caller must balance withphar_entry_delref. Two error paths in the content-write block jumped tofinish:viagoto, bypassing thephar_entry_delrefcall. Thefinish:label sits after the delref on the success path. Addedphar_entry_delref(data)before eachgoto finishin those branches.Fixes #21798